Add more tests to blog post #481

yceruto · 2017-02-18T01:15:24Z

Testing new blog post
Testing show blog post (admin)
Testing add a comment to post
Changing some PostFixtures methods to static for reusability
Simplify and reordering some tests

voronkovich · 2017-02-18T12:20:58Z

src/AppBundle/DataFixtures/ORM/PostFixtures.php

@@ -184,9 +183,9 @@ private function getPhrases()
        ];
    }

-    private function getRandomPostTiles()
+    public static function getRandomPostTiles()


@yceruto, there is a typo in the method's name here. Should be getRandomPostTitles.

Thanks, Fixed!

voronkovich · 2017-02-18T16:50:46Z

tests/AppBundle/Controller/BlogControllerTest.php

+        ]);
+
+        /** @var Post $post */
+        $post = $client->getContainer()->get('doctrine')->getRepository(Post::class)->find(1);


@yceruto, you could use an EntityManager#find method:

$post = $client->getContainer()->get('doctrine')->find(Post::class, 1);

Sorry your suggestion does not works.

@yceruto, Sorry for confusing you! I mixed up the doctrine service and an entity manager. :(

bocharsky-bw · 2017-02-23T08:08:38Z

tests/AppBundle/Controller/Admin/BlogControllerTest.php

            'PHP_AUTH_USER' => 'john_user',
            'PHP_AUTH_PW' => 'kitten',
        ]);

        $client->request($httpMethod, $url);
-        $this->assertSame($statusCode, $client->getResponse()->getStatusCode());
+        $this->assertTrue($client->getResponse()->isForbidden());


You made it less strict, but I don't think it makes sense in tests.

In this case, isForbidden() only checks the 403 status code ... so the code it's technically equivalent...

Yes, in this case it's technically equivalent.

bocharsky-bw · 2017-02-23T08:09:23Z

tests/AppBundle/Controller/Admin/BlogControllerTest.php

@@ -70,7 +70,7 @@ public function testAdminBackendHomePage()
        $client = $this->getAdminClient();

        $crawler = $client->request('GET', '/en/admin/post/');
-        $this->assertSame(Response::HTTP_OK, $client->getResponse()->getStatusCode());
+        $this->assertTrue($client->getResponse()->isSuccessful());


The same here, I think we want to know that status code is exactly 200 OK

... however, here the isSuccessful() method is too liberal:

public function isSuccessful() { return $this->statusCode >= 200 && $this->statusCode < 300; }

I agree with @bocharsky-bw and I think we should revert this change and go back to Response::HTTP_OK

👍 reverting....

bocharsky-bw · 2017-02-23T08:14:19Z

src/AppBundle/DataFixtures/ORM/PostFixtures.php

@@ -108,7 +107,7 @@ private function getRandomTags($numTags = 0)
        return $tags;
    }

-    private function getPostContent()
+    public static function getPostContent()


👎 I'd rather suggest to use PHP Traits instead of making private methods public static or do simple inheritance. We should always avoid public methods which we have to care about later.

👍 for Trait

javiereguiluz · 2017-02-23T11:09:22Z

tests/AppBundle/Controller/Admin/BlogControllerTest.php

-        $post = $client->getContainer()->get('doctrine')->getRepository(Post::class)->find(1);
-        $this->assertNull($post);
+        /** @var Post $post */
+        $post = $client->getContainer()->get('doctrine')->getRepository(Post::class)->find(31);


The 31 id should never fail ... but maybe a findByTitle($postTitle) would make the code more robust?

javiereguiluz · 2017-02-23T11:12:29Z

tests/AppBundle/Controller/Admin/BlogControllerTest.php

            'PHP_AUTH_USER' => 'john_user',
            'PHP_AUTH_PW' => 'kitten',
        ]);

        $client->request($httpMethod, $url);
-        $this->assertSame($statusCode, $client->getResponse()->getStatusCode());
+        $this->assertTrue($client->getResponse()->isForbidden());


In this case, isForbidden() only checks the 403 status code ... so the code it's technically equivalent...

javiereguiluz · 2017-02-23T11:13:25Z

tests/AppBundle/Controller/Admin/BlogControllerTest.php

@@ -70,7 +70,7 @@ public function testAdminBackendHomePage()
        $client = $this->getAdminClient();

        $crawler = $client->request('GET', '/en/admin/post/');
-        $this->assertSame(Response::HTTP_OK, $client->getResponse()->getStatusCode());
+        $this->assertTrue($client->getResponse()->isSuccessful());


... however, here the isSuccessful() method is too liberal:

public function isSuccessful() { return $this->statusCode >= 200 && $this->statusCode < 300; }

I agree with @bocharsky-bw and I think we should revert this change and go back to Response::HTTP_OK

yceruto · 2017-02-23T14:14:28Z

@javiereguiluz @bocharsky-bw done!

Now the fixtures and tests using a FixturesTrait for common methods.
and all controller test case now extending from ControllerTestCase to use common decriptive methods and strict assertions for response instances.

Status: Need Reviews :)

yceruto

Perhaps Javier could help me with a nice description here :)

yceruto · 2017-02-23T14:41:14Z

tests/ControllerTestCase.php

+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\HttpFoundation\Response;
+
+class ControllerTestCase extends WebTestCase


Missing description

yceruto · 2017-02-23T14:41:46Z

tests/FixturesTrait.php

+
+namespace Tests;
+
+trait FixturesTrait


Missing description

javiereguiluz · 2017-02-24T12:10:28Z

I'm not that sure about tests/ControllerTestCase.php. For experienced Symfony developers makes sense ... but for newcomers? They probably see that and think ... "mmm, it's pretty complex to create just one simple functional test ... first I need to create this special ControllerTestCase, then I need to create my tests, etc."

I'm not asking to removing this (yet) ... but let's think a bit about it. Thanks!

yceruto · 2017-02-24T13:32:10Z

This ControllerTestCase is just an utility class to make tests more descriptive and simple, it's more related to basic POO inheritance and DRY (no Symfony/funtional Tests related).

I think it's good to show to newcomers good practices related to how to simplify their tests too. For newcomers of "Symfony framework" it should be easy to understand I think, anyway a nice class description could clarify possible misinterpretations,

EDIT: but I'm fine with removing this class, if you prefer :) (or part of it)

yceruto · 2017-02-24T14:08:58Z

Maybe I could remove the response assertions and leave only the client creation methods, what do you think?

yceruto · 2017-03-14T09:39:45Z

Last changes:

Moved response assert to the tests methods again
Changed controller test inheritance by trait helper
Added comments

yceruto · 2017-03-14T14:25:32Z

Rebased (#502 conflicts resolved)

javiereguiluz · 2017-03-14T15:26:50Z

@yceruto thanks for all the changes and for rebasing, etc. There are a lot of things that I like in this PR, so I'm going to merge it now. However, there are some things that I don't like because it's not a practice promoted by Symfony (although your proposal works perfectly and it's technically correct). So, I'm going to create a PR soon after to revert some changes. Thanks!

This PR was merged into the master branch. Discussion ---------- Add more tests to blog post * Testing new blog post * Testing show blog post (admin) * Testing add a comment to post * Changing some `PostFixtures` methods to static for reusability * Simplify and reordering some tests Commits ------- 2700136 Add more tests

@yceruto

This PR was merged into the master branch. Discussion ---------- Removed ControllerTestTrait In #481, @yceruto introduced some nice features. I like them all ... but one of them is not compatible with the way Symfony promotes to do things. The `ControllerTestTrait` is a PHP trait with some helper methods to reduce the boilerplate code in tests. Even if the solution is technically correct, we don't use it in the official Symfony docs. This can create confusion for new developers studying this app and reading the docs. I propose to revert this feature in this PR ... and later we can discuss about ways to improve this without using traits. Thanks! Commits ------- 17f9a10 Removed ControllerTestTrait

voronkovich reviewed Feb 18, 2017

View reviewed changes

lboynton mentioned this pull request Feb 21, 2017

Fix typo in method name #484

Closed

bocharsky-bw reviewed Feb 23, 2017

View reviewed changes

javiereguiluz added Feature FINISHED labels Feb 23, 2017

javiereguiluz reviewed Feb 23, 2017

View reviewed changes

javiereguiluz requested changes Feb 23, 2017

View reviewed changes

javiereguiluz removed the FINISHED label Feb 23, 2017

yceruto commented Feb 23, 2017

View reviewed changes

Add more tests

2700136

javiereguiluz approved these changes Mar 14, 2017

View reviewed changes

javiereguiluz merged commit 2700136 into symfony:master Mar 14, 2017

yceruto deleted the tests branch March 14, 2017 15:39

javiereguiluz mentioned this pull request Mar 14, 2017

Removed ControllerTestTrait #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more tests to blog post #481

Add more tests to blog post #481

yceruto commented Feb 18, 2017

voronkovich Feb 18, 2017

yceruto Feb 18, 2017

voronkovich Feb 18, 2017

yceruto Feb 22, 2017

voronkovich Feb 22, 2017

bocharsky-bw Feb 23, 2017

javiereguiluz Feb 23, 2017

yceruto Feb 23, 2017

bocharsky-bw Feb 23, 2017

javiereguiluz Feb 23, 2017

yceruto Feb 23, 2017

bocharsky-bw Feb 23, 2017

yceruto Feb 23, 2017

javiereguiluz Feb 23, 2017

yceruto Feb 23, 2017

javiereguiluz Feb 23, 2017

javiereguiluz Feb 23, 2017

yceruto commented Feb 23, 2017 •

edited

Loading

yceruto left a comment •

edited

Loading

yceruto Feb 23, 2017

yceruto Feb 23, 2017

javiereguiluz commented Feb 24, 2017

yceruto commented Feb 24, 2017 •

edited

Loading

yceruto commented Feb 24, 2017

yceruto commented Mar 14, 2017 •

edited

Loading

yceruto commented Mar 14, 2017 •

edited

Loading

javiereguiluz commented Mar 14, 2017

Add more tests to blog post #481

Add more tests to blog post #481

Conversation

yceruto commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto commented Feb 23, 2017 • edited Loading

yceruto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Feb 24, 2017

yceruto commented Feb 24, 2017 • edited Loading

yceruto commented Feb 24, 2017

yceruto commented Mar 14, 2017 • edited Loading

yceruto commented Mar 14, 2017 • edited Loading

javiereguiluz commented Mar 14, 2017

yceruto commented Feb 23, 2017 •

edited

Loading

yceruto left a comment •

edited

Loading

yceruto commented Feb 24, 2017 •

edited

Loading

yceruto commented Mar 14, 2017 •

edited

Loading

yceruto commented Mar 14, 2017 •

edited

Loading